Skip to content

Modify SqlStreamingXml XmlWriter to internally use a MemoryStream#3974

Open
jimhblythe wants to merge 3 commits intodotnet:mainfrom
jimhblythe:issue-1877
Open

Modify SqlStreamingXml XmlWriter to internally use a MemoryStream#3974
jimhblythe wants to merge 3 commits intodotnet:mainfrom
jimhblythe:issue-1877

Conversation

@jimhblythe
Copy link

@jimhblythe jimhblythe commented Feb 20, 2026

Modify SqlStreamingXml XmlWriter to internally use a MemoryStream instead of a StringBuilder.

Note: UTF8Encoding(false) addition in s_writerSettings is consistent with prior default used within StringWriter/StringBuilder

Issues

Fixes #1877 to be O(n)

Testing

Added 2 new Manual tests to ensure linear behavior for single large node, and secondary validation for multiple nodes
image

@jimhblythe
Modify SqlStreamingXml XmlWriter to internally use a MemoryStream ins…
3cc826e 
…tead of a StringBuilder. (dotnet#1877)

Note: UTF8Encoding(false) addition in s_writerSettings is consistent with prior default used within StringWriter/StringBuilder
@jimhblythe jimhblythe requested a review from a team as a code owner February 20, 2026 21:03
@github-project-automation github-project-automation bot moved this to To triage in SqlClient Board Feb 20, 2026
@jimhblythe
Copy link
Author

jimhblythe commented Feb 20, 2026

@dotnet-policy-service agree

@mdaigle mdaigle moved this from To triage to Needs Response in SqlClient Board Feb 24, 2026
@jimhblythe
Fix MemoryStream to allow appending when GetChars request spans multi…
e2dec42 
…ple elements

Enhance comments within SqlStreamingXml
Extend Manual tests to fully cover GetChars
WriteXmlElement includes uncovered paths not accessible for SQL XML column types which normalize Whitespace, CDATA, EntityReference, XmlDeclaration, ProcessingInstruction, DocumentType, and Comment node types
@paulmedynski
Copy link
Contributor

paulmedynski commented Feb 27, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 27, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski self-assigned this Feb 27, 2026
@paulmedynski paulmedynski moved this from Needs Response to In review in SqlClient Board Feb 27, 2026
paulmedynski
paulmedynski reviewed Feb 27, 2026
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - thanks for this optimization and expanded test coverage! Just one question.

@paulmedynski
Copy link
Contributor

paulmedynski commented Feb 27, 2026

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 27, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski added this to the 7.0.0 milestone Feb 27, 2026
@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.59%. Comparing base (a68e00f) to head (9f44ae8).
⚠️ Report is 24 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (a68e00f) and HEAD (9f44ae8). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (a68e00f) HEAD (9f44ae8)
netfx 2 0
netcore 2 0
addons 2 0
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #3974       +/-   ##
===========================================
- Coverage   75.22%   64.59%   -10.63%     
===========================================
  Files         266      282       +16     
  Lines       42932    66080    +23148     
===========================================
+ Hits        32294    42683    +10389     
- Misses      10638    23397    +12759
Flag Coverage Δ
PR-SqlClient-Project 64.59% <100.00%> (?)
addons ?
netcore ?
netfx ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mdaigle added a commit that referenced this pull request Mar 5, 2026
@mdaigle
Add non-ASCII XML streaming tests for SqlDataReader.GetChars
4ee4fec 
Add tests that verify SqlDataReader.GetChars correctly handles non-ASCII
characters when reading XML columns with SequentialAccess:

- GetChars_NonAsciiContent: Latin accented characters (2-byte UTF-8)
- GetChars_NonAsciiContent_BulkRead: Bulk read path with accented chars
- GetChars_CjkContent: CJK characters (3-byte UTF-8)

These tests establish a baseline for correct behavior on main before
PR #3974 (issue #1877) refactors SqlStreamingXml internals.
@mdaigle mdaigle mentioned this pull request Mar 5, 2026
Draft
@mdaigle
Copy link
Contributor

mdaigle commented Mar 5, 2026 •
edited
Loading

@jimhblythe can you take a look at the tests in #4005 and #4008? They seem to suggest that this introduces a regression for non-ascii characters. You can check the pipeline results to see the test outcomes. I'd like to see some tests like that included in this PR as well, please 🙏

@jimhblythe
Copy link
Author

jimhblythe commented Mar 7, 2026

@jimhblythe can you take a look at the tests in #4005 and #4008? They seem to suggest that this introduces a regression for non-ascii characters. You can check the pipeline results to see the test outcomes. I'd like to see some tests like that included in this PR as well, please 🙏

I will additionally including a fourth test to verify surrogate pairs:

        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
        public static void GetChars_SurrogatePairContent()
        {
            SqlConnection connection = new(DataTestUtility.TCPConnectionString);
            // Surrogate Pair characters: 4 bytes each in UTF-8
            string xml = "<data>\U0001F600\U0001F525\U0001F680</data>";

Actual: "<data>😀🔥🚀</data>"

The test shows these worked with prior code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulmedynski paulmedynski paulmedynski approved these changes

+1 more reviewer

@Wraith2 Wraith2 Wraith2 left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

@paulmedynski paulmedynski

Labels

None yet

Projects

SqlClient Board
Status: In review

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

O(N^2) performance when reading XML with SequentialAccess

4 participants

@jimhblythe @paulmedynski @mdaigle @Wraith2